refactor(proto)!: use well-known time types - #3113
Conversation
Signed-off-by: Derek Carr <decarr@redhat.com>
|
/ok to test 62e0845 |
drew
left a comment
There was a problem hiding this comment.
The overall design is sound: replacing scalar time fields with protobuf well-known types, reserving retired tags, and transactionally migrating persisted payloads. I found several behavioral regressions that need to be addressed before merge. The branch is also 23 commits behind main; GitHub currently reports unresolved conflicts in provider.rs, sandbox.rs, and ssh_sessions.rs.
-
Default
sandbox execimmediately times out. Before this PR, the CLI's documented--timeout 0bypassed timeout handling. The PR always sendsSome(Duration::ZERO), causing both noninteractive and interactive commands to entertokio::time::timeout(Duration::ZERO, ...)and normally return exit 124. Details:crates/openshell-cli/src/run.rs:1580-1585,:1958-1963;crates/openshell-server/src/grpc/sandbox.rs:2053-2068,:2134-2149. -
Default inference routes get a zero-second timeout. Before this PR,
--timeout 0selected the 60-second default. The PR persists an explicit zero duration, which the supervisor treats as a real zero timeout. Ordinary inference requests can therefore fail immediately. Partial updates also replace an absent timeout with zero and truncate fractional durations. Details:crates/openshell-cli/src/run.rs:5489-5492,:5557-5583;crates/openshell-supervisor-network/src/inference_routes.rs:313-317. -
Credential expiry can no longer be cleared. Before this PR,
credential_expires_at_ms[key] = 0removed an existing expiry. The new timestamp map has no per-key tombstone: omitting a key is a no-op, while epoch is a real, already-expired timestamp. The proto comment claiming an absent map entry removes the expiry cannot be implemented by the current merge. Details:proto/openshell.proto:1471-1477;crates/openshell-server/src/grpc/provider.rs:707-717;sdk/go/openshell/v1/internal/converter/provider.go:84-87. -
Previously valid persisted conditions can block gateway startup. The old
last_transition_timewas an unconstrained driver-provided string. The migration now requires every nonempty value to parse as RFC 3339 and aborts the entire startup transaction otherwise. A custom driver value accepted by the previous release can therefore make the upgraded gateway unavailable. Details:crates/openshell-server/src/persistence/legacy_time_wire.rs:113-125. -
Provider-profile durations bypass WKT validation and lose precision. Raw profile imports copy only
Duration.seconds, ignoring nanos and malformed sign/range combinations. For example,0.5sbecomes zero and an invalid{seconds:1,nanos:-1}becomes one second. The Go converter additionally casts negative signed values touint64, turning validation errors into absent/default durations. Details:crates/openshell-providers/src/profiles.rs:1026-1039,:1095-1111;sdk/go/openshell/v1/internal/converter/profile.go:191-198,:233-242. -
Invalid log filters become unfiltered queries. Before this PR, the integer
since_mscould not be structurally malformed.GetSandboxLogsnow silently converts an invalid timestamp to zero and returns unfiltered history instead ofINVALID_ARGUMENT;WatchSandboxcorrectly rejects the same input. Details:crates/openshell-server/src/grpc/policy.rs:4109-4119; comparecrates/openshell-server/src/grpc/sandbox.rs:973-979. -
The upgrade guide omits external protobuf peers. Compute drivers, credential drivers, middleware services, and sandbox supervisors also need coordinated upgrades. An old credential driver's tag-3 expiry is ignored by the new gateway, which then treats the credential as non-expiring. Details:
docs/reference/protobuf-time-types.mdx:28-30;proto/credential_driver.proto:115-123;crates/openshell-server/src/credentials.rs:692-704. -
Negative TypeScript exec timeouts become unlimited. Before this PR, encoding
-1into the olduint32field failed. The new helper maps every nonpositive or nonfinite value to absence, allowing an invalid negative timeout to run indefinitely. Details:sdk/typescript/src/client.ts:35-37,:708,:789. -
Agent guidance is stale. The TUI development skill still documents
SandboxLogLine.timestamp_msandGetSandboxLogsRequest.since_ms, so agents following it will generate obsolete code. Details:.agents/skills/tui-development/SKILL.md:493-494.
Branch CI is green, but all E2E lanes were skipped and the checks predate integration with current main. Focused Rust compilation and time/migration tests passed; Go, TypeScript, and Python SDK tests also passed. The missing cases are the sentinel, invalid-duration, expiry-removal, and legacy-string scenarios above.
|
Label |
Summary
Replace public protobuf timestamp and duration scalars with the standard
google.protobuf.Timestampandgoogle.protobuf.Durationtypes. This gives API consumers consistent time semantics, preserves sub-second precision, and distinguishes absent optional values from zero-valued times and durations.Related Issue
Closes #3052
Changes
Testing
mise run pre-commitpassesChecklist